-
Notifications
You must be signed in to change notification settings - Fork 9
chore: remove fluent api and clean up examples #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two notes:
- Three examples were removed entirely; as we currently use these as informal manual testing, I think they should stay (or be removed in a separate PR)
- I see you deleted
U.throwIfError
; why? IMO it is much cleaner with it than the expanded version
util/error.ts
Outdated
@@ -4,10 +4,3 @@ export function ErrorCtor<Name extends string>(name: Name) { | |||
override readonly name = name; | |||
}; | |||
} | |||
|
|||
export function throwIfError<T>(value: T): Exclude<T, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? I think this is a regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in 3 tests... an instanceof
check is slightly larger, but it's clear what's happening in place (without following the import). In the case of examples: we want it to be extremely clear to users what's going on. My thinking is that throwIfError
comes with an unworthy trade off: the cognitive load of bringing throwIfError
into scope (and figuring out what it does) isn't worth the size savings. That being said, I agree: avoids the local binding on which to perform the instanceof
check... are we (am I) overthinking this? Should we revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in 3 tests... and pretty much every single example. While we don't use it much in capi (since we tend to return results), I think it's a very useful utility for users and makes the examples a bit cleaner.
In terms of cognitive overhead, once one knows what it does, that's not a problem. Whereas with the expanded version, one has to read it to make sure it's the boilerplate and not doing something weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! I agree. Reverting now.
I hastily removed all fluent examples (whoops). I've added non-fluent equivalents of the deleted examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed up on the throwIfError
, but looks good otherwise
Fluent API is no longer necessary (see #226). Cleaned up examples as well.